Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added prometheus metrics #366

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Added prometheus metrics #366

merged 1 commit into from
Mar 2, 2022

Conversation

neufeldtech
Copy link
Contributor

@neufeldtech neufeldtech commented Feb 25, 2022 •
edited
Loading

Implements #365

Added two different collectors for Prometheus metrics that can be enabled independently of each other with CLI flags. These metrics are served from /metrics HTTP api endpoint when at least one of the collectors is enabled.

  1. Runtime metrics -runtime-metrics
    These runtime metrics are 'common metrics' that come from these collectors:
  1. Proxy Metrics -proxy-metrics
    These are toxiproxy-specific metrics that expose the internal state/metrics of toxiproxy. See METRICS.md for description of these metrics.

The current proxy metrics look like this:

# HELP toxiproxy_proxy_received_bytes_total The total number of bytes received on a given proxy link in a given direction
# TYPE toxiproxy_proxy_received_bytes_total counter
toxiproxy_proxy_received_bytes_total{direction="downstream",listener="[::]:8080",proxy="my-proxy",upstream="httpbin.org:80"} 463
toxiproxy_proxy_received_bytes_total{direction="upstream",listener="[::]:8080",proxy="my-proxy",upstream="httpbin.org:80"} 89

# HELP toxiproxy_proxy_sent_bytes_total The total number of bytes sent on a given proxy link in a given direction
# TYPE toxiproxy_proxy_sent_bytes_total counter
toxiproxy_proxy_sent_bytes_total{direction="downstream",listener="[::]:8080",proxy="my-proxy",upstream="httpbin.org:80"} 463
toxiproxy_proxy_sent_bytes_total{direction="upstream",listener="[::]:8080",proxy="my-proxy",upstream="httpbin.org:80"} 89

@neufeldtech neufeldtech marked this pull request as ready for review February 27, 2022 18:36
miry
miry reviewed Feb 28, 2022
View reviewed changes
README.md Show resolved Hide resolved
miry
miry reviewed Feb 28, 2022
View reviewed changes
link.go Outdated Show resolved Hide resolved
miry
miry reviewed Feb 28, 2022
View reviewed changes
metrics/metrics.go Outdated Show resolved Hide resolved
@miry miry modified the milestone: 2.4.0 Feb 28, 2022
miry
miry reviewed Feb 28, 2022
View reviewed changes
link.go Outdated Show resolved Hide resolved
@miry
Copy link
Contributor

miry commented Feb 28, 2022 •
edited
Loading

I think by default the metrics should be disabled. We should allow to enable metrics.
It would help to have initialization part for metrics, than could be configured.

Some example that looks like covers my comments: https://developpaper.com/add-prometheus-monitoring-indicators-for-go-applications/

@neufeldtech
Copy link
Contributor Author

I think by default the metrics should be disabled. We should allow to enable metrics. It would help to have initialization part for metrics, than could be configured.

Some example that looks like covers my comments: https://developpaper.com/add-prometheus-monitoring-indicators-for-go-applications/

It would help to have initialization part for metrics, than could be configured.

I will refactor to make metrics optional (feature flagged) via a command line arg (and likely an environment variable as well, for easy Kubernetes compatibility).

@neufeldtech neufeldtech requested a review from miry February 28, 2022 19:09
@neufeldtech
Copy link
Contributor Author

We might need to discuss how to integrate this a little further - I just found another instance of NewProxy() where we don't have access to the server parent struct to get the metrics config data

https://github.com/Shopify/toxiproxy/blob/master/proxy_collection.go#L97-L101

Eagerod
Eagerod reviewed Feb 28, 2022
View reviewed changes
api.go Outdated Show resolved Hide resolved
Eagerod
Eagerod reviewed Feb 28, 2022
View reviewed changes
metrics/common.go Outdated Show resolved Hide resolved
Eagerod
Eagerod reviewed Feb 28, 2022
View reviewed changes
metrics/proxy.go Outdated Show resolved Hide resolved
miry
miry reviewed Mar 1, 2022
View reviewed changes
api_test.go Show resolved Hide resolved
miry
miry reviewed Mar 1, 2022
View reviewed changes
link.go Outdated Show resolved Hide resolved
miry
miry reviewed Mar 1, 2022
View reviewed changes
metrics.go Outdated Show resolved Hide resolved
miry
miry reviewed Mar 1, 2022
View reviewed changes
scripts/test-e2e Show resolved Hide resolved
miry
miry approved these changes Mar 1, 2022
View reviewed changes
Copy link
Contributor

@miry miry left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comments left and linting errors. Overall it looks awesome.

neufeldtech
neufeldtech commented Mar 1, 2022
View reviewed changes
cmd/server/server.go Outdated Show resolved Hide resolved
@neufeldtech neufeldtech merged commit 5cfb64c into master Mar 2, 2022
@neufeldtech neufeldtech deleted the prometheus branch March 2, 2022 14:10
@neufeldtech neufeldtech mentioned this pull request Mar 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eagerod Eagerod Eagerod left review comments

@miry miry miry approved these changes

Assignees

@neufeldtech neufeldtech

Labels
observability Toxiproxy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@neufeldtech @miry @Eagerod